The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #94 (Allow configuring target block time for a signet by benthecarman)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Thanks for kicking CI @ajtowns. I fixed the missing annotations that DEBUG_LOCKORDER complained about.

The consensus changes, with respect to their specification in BIP54, look good to me:

• 84e1043 validation: make BIP54 sigops check consensus-critical
• 87e9e65 consensus: prevent timewarp attacks with a 2h grace period
• c6c1b18 consensus: prevent negative difficulty adjustment intervals
• 00745a2 consensus: mandate coinbase transactions be timelocked to block height
• 8ee48e4 consensus: make 64 bytes transactions invalid

I think this PR is well organised (starting at ---). Each consensus change is first prepared, done in a minimal commit, and then followed by new tests.

I'll study the test changes in more detail later.

Finished a first round of review of the code for the 4 consensus fixes:

• “difficulty adjustment exploits”
• “long block validation time”
• “merkle tree malleability with 64-byte transaction”
• “possibility of duplicate coinbase transactions”

Will review more the fixes at the conceptual level and the breadth / depth of their respective test coverage.

That said, the cache doesn't seem necessary, just avoiding serializing the entire block as json seems to improve things ~60x for me

Would simply adding the block's coinbase's locktime to the output of verbosity-1 getblock be sufficient then?

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index bbe401fcc6c..78062c74e6f 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -183,6 +183,7 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn
     result.pushKV("strippedsize", (int)::GetSerializeSize(TX_NO_WITNESS(block)));
     result.pushKV("size", (int)::GetSerializeSize(TX_WITH_WITNESS(block)));
     result.pushKV("weight", (int)::GetBlockWeight(block));
+    result.pushKV("cblocktime", block.vtx.at(0)->nLockTime);
     UniValue txs(UniValue::VARR);
 
     switch (verbosity) {
@@ -724,6 +725,7 @@ static RPCHelpMan getblock()
                     {RPCResult::Type::NUM, "version", "The block version"},
                     {RPCResult::Type::STR_HEX, "versionHex", "The block version formatted in hexadecimal"},
                     {RPCResult::Type::STR_HEX, "merkleroot", "The merkle root"},
+                    {RPCResult::Type::NUM, "locktime", "The coinbase transaction's locktime value"},
                     {RPCResult::Type::ARR, "tx", "The transaction ids",
                         {{RPCResult::Type::STR_HEX, "", "The transaction id"}}},
                     {RPCResult::Type::NUM_TIME, "time",       "The block time expressed in " + UNIX_EPOCH_TIME},

I've tested this on mainnet with your snippet from earlier and it's reasonably performant.

On the coinbase transaction and requesting the nLocktime to be set to the current block height minus one and the nSequence field must not be equal to 0xff_ff_ff_ff, I'm still thinking the second check is unecessary. The first check is ensuring that the transaction is always final (2nd check of IsFinal()). I’ve yet to verify with a test, but post-BIP54 the 3rd check in IsFinal() should never be reach by validating a coinbase transaction on its own.

#99 (comment)

https://groups.google.com/g/bitcoindev/c/6TTlDwP2OQg

Per se, unless I'm missing something, I don't think the value of the nSequence field needs to be set to a precise value and the whole field could be even “free" up field either for (a) for an extra nonce or (b) as a space where to fit a new commitment structure in the future (and that's particularly interesting as it would work even if there are no segwit txn in the block cf. BIP141). cc @darosior

Would simply adding the block's coinbase's locktime to the output of verbosity-1 getblock be sufficient then?

Yeah, seems like -- it takes about 3 times as long as the dedicated RPC for me in a quick test, but that's good enough for how much simpler it is. I found "cblocktime" confusing (CBlock time?), maybe tx0_locktime? Also should report whether nseq is final or not. Could also just report "bip54_coinbase_readiness": true/false.

I messed around with the dedicated approach some more; it looks like if you add a dedicated ReadBlockCoinbaseOnly method to block storage, which just reads the header and coinbase tx, but not the rest of the block, then the dedicated rpc gets a massive speedup. My figures are:

• just using the RPCs we have now: 2 retarget periods takes about 2 hours to scan
• a couple of lines of code to add reporting it to getblock 1: 2 retarget periods takes about 10 minutes to scan
• dedicated rpc: 2 retarget periods takes about 4 minutes to scan
• dedicated rpc loading block headers and coinbase tx only: 2 retarget periods takes under 5 seconds to scan

I'll update I've updated my branch, but the getblock 1 approach seems like the sweet spot. (It's probably also faster if your node doesn't have quite as convoluted a setup as mine)

I found "cblocktime" confusing (CBlock time?), maybe tx0_locktime?

Indeed, it reads like "c block time", cb_locktime should work.

Also should report whether nseq is final or not. Could also just report "bip54_coinbase_readiness": true/false.

Ideally we keep it generic enough so it can be PR'd to Bitcoin Core indepedent of BIP54.

Maybe add a coinbase field to getblock with (a guaranteed to be small subset of) fields similar to the CoinbaseTx struct from bitcoin#33819 : version, sequence and lock_time. Could add an is_final bool for convenience, given its relevance.

Update: bitcoin#34512

ACK 55cd9a5 -- this looks good to me i think, and should be ready to merge

#99 (comment)

https://groups.google.com/g/bitcoindev/c/6TTlDwP2OQg

Per se, unless I'm missing something, I don't think the value of the nSequence field needs to be set

@ariard i have replied to you on the mailing list. TL;DR: it seems my explanation was lacking and you missed the purpose for making sure nSequence is never final.

to a precise value

Note also we don't constrain it to be a specific value. On the contrary it can be anything as long it's not the specific final value.

@ariard i have replied to you on the mailing list. TL;DR: it seems my explanation was lacking and you missed the purpose for making sure nSequence is never final.

@darosior I think I got your idea with the assumption that BIP54 can be buried in the future, and the check to be applied on all historical coinbase transactions, therefore ensuring any old coinbase cannot be used to generate a post-BIP54 action height identifier collision.

Replied, I’m still thinking you can make a simpler assumption by just relying on the PoW ordering mechanism itself. There can be only and only one block valid at a given chain height.

Note also we don't constrain it to be a specific value. On the contrary it can be anything as long it's not the specific final value.

Yes, I see it doesn’t have to be set to a specific value. See the point on protecting the coinbase nSequence field. It’s of independent interest, that the check is kept or not.

@darosior I think I got your idea with the assumption that BIP54 can be buried in the future, and the check to be applied on all historical coinbase transactions,

Historical coinbase transactions on mainnet, testnet3, testnet4 and signet all fail to comply with bip54 rules, so that approach isn't possible.

Historical coinbase transactions on mainnet, testnet3, testnet4 and signet all fail to comply with bip54 rules, so that approach isn't possible.

Okay so up to Darosior what he meant here on the ml:

"For instance, if BIP 54 activation height ever gets buried in Bitcoin Core, the BIP 30 check could just be disabled past
this height instead of having to figure out if we are on a chain that contains the historical BIP 34 activation block hash [3].

So strictly a buried activation height, nothing like a retroactive validation from genesis like it was done for BIP30.

Point is the additional check in nsequence field is superflous imho.

The issue is what happens if there is a reorg back to a point where neither BIP 34 or BIP 54 are active. At that point, BIP 30 checks must be enabled. With the nsequence check, those checks no longer need to be performed after BIP 54 is enabled because no coinbase that complies with its rules can have ever previously appeared in the blockchain. Also, this discussion should be on the mailing list, not here.